-
Notifications
You must be signed in to change notification settings - Fork 12.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parse quoted constructors as constructors, not methods #31949
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably needs changes to forEachChild
, transform visitor and factory APIs, too.
src/compiler/parser.ts
Outdated
return tryParse(() => { | ||
const stringLiteral = parseLiteralNode(); | ||
if (isStringLiteral(stringLiteral) | ||
&& stringLiteral.text === "constructor" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the string literal allowed to contain escapes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, escapes are handled in the lexical grammar. text
is the unescaped version, so it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked because I wasn't sure if the spec allowed escapes in the string literal. Tested it with Chrome and it recognizes it as constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I’m not completely following—can you show me exactly what you tried with Chrome?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test whether ES6 classes allow escapes in the sting literal "constructor"
I executed the following code:
new (class C {"\x63onstructor"(param) {console.log(param)}})(1)
I was just curious because "use strict"
is only recognized if it does not contain escapes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled escaped constructors in the latest commit. I think this is ready now.
src/compiler/parser.ts
Outdated
&& token() === SyntaxKind.OpenParenToken | ||
) { | ||
node.kind = SyntaxKind.Constructor; | ||
node.name = stringLiteral; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regular constructors don't have a name
property. To avoid polymorphism you could
- explicitly set it to undefined in parseConstructorDeclaration
- set it after parseConstructorDeclarationEnd (not preferred as it still introduces a new object shape)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True; I guess we don’t need to set that property. I was setting it just because we already have the node and it would allow you to disambiguate the exact structure of the declaration if needed, but that should be covered by getChildren()
.
Should be labeled as breaking change to the API |
src/compiler/parser.ts
Outdated
@@ -5861,6 +5879,13 @@ namespace ts { | |||
return parseAccessorDeclaration(<AccessorDeclaration>node, SyntaxKind.SetAccessor); | |||
} | |||
|
|||
if (token() === SyntaxKind.StringLiteral) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just check the token text here and call parseConstructorDeclaration, and have parseConstructorDeclaration handle whether its a string or keyword? The public fields proposal disallows public fields with the name constructor
or "constructor"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because we already give a better error at the grammar checking stage for fields named constructor
—it says Classes may not have a field named 'constructor'.
instead of '(' expected
.
I think if we want to preserve the quoted string constructor in refactors, emit, etc., the easiest thing to do is probably to keep the Alternatively, if we want to drop the Curious to get @rbuckton’s opinion here. |
I'm not sure how important it is to preserve whether the constructor name was quoted. |
I would say it's quite unimportant - it's not possibly subject to minification, which is the only thing people seem to care about in terms of preserving quotedness |
Fixes #31020, part 2. We now properly parse
class A { "constructor"() {} }
as a constructor, not as a method. Part 1 was issuing an error to warn people that the behavior was changing: #31119